Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code formatter #305

Merged
merged 2 commits into from
Feb 11, 2022
Merged

Code formatter #305

merged 2 commits into from
Feb 11, 2022

Conversation

seriyps
Copy link
Collaborator

@seriyps seriyps commented Jan 26, 2022

Just a proof of concept to start the discussion.

Pros:

  • we are going to have consistent and modern code style

Cons:

  • git blame will be confusing by default (unless one does git config --global blame.ignoreRevsFile .git-blame-ignore-revs, which is quite common solution)
  • already opened PRs and forks would get quite big merge conflicts (could be solved by running rebar3 fmt over the fork's source tree). But right now we don't have that many open PRs.

@mworrell
Copy link
Collaborator

For me this is easier to read, so I'm for the change.

@mworrell
Copy link
Collaborator

Maybe it is good to rebase and merge #235 before we merge this.

Other PRs don't seem ready to merge.

@seriyps
Copy link
Collaborator Author

seriyps commented Jan 27, 2022

I'll rebase

@mworrell
Copy link
Collaborator

mworrell commented Feb 1, 2022

Looks good to me!

@arjan are you ok?

@mworrell
Copy link
Collaborator

@seriyps Go ahead and merge.

Then we can ask @Maria-12648430 to check her pull request #307

@seriyps seriyps merged commit ab680a7 into gen-smtp:master Feb 11, 2022
@seriyps
Copy link
Collaborator Author

seriyps commented Feb 11, 2022

Thanks! 🎉

@Maria-12648430
Copy link
Contributor

Then we can ask @Maria-12648430 to check her pull request #307

Heeey! Don't make me your guinea pig 😅 j/k

There are some merge conflicts, as expected. I'll have to look at them more closely later, busy right now 😉

@mworrell
Copy link
Collaborator

Sorry 😇

iu

@cw789
Copy link
Contributor

cw789 commented Feb 12, 2022

Glad this got merged. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants